fix: fetch plugin documentation - #26
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced HTTP-based plugin docs retrieval with a Git-based approach using GitPython; added Changes
Sequence Diagram(s)sequenceDiagram
participant Collector as Collector (source/collect_plugins.py)
participant Git as GitPython (remote repo)
participant FS as Filesystem
participant Renderer as Docs Renderer
Collector->>Git: clone(repo_url, branch)
Git-->>FS: write cloned files to tmpdir
Collector->>FS: open docs/{section}.md
FS-->>Collector: file content or None
Collector->>Renderer: render docs when content present
Renderer-->>Collector: rendered output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pixi.toml`:
- Line 23: The build contract is missing the system git executable required by
GitPython: update pixi.toml to declare the git runtime dependency (in addition
to gitpython) so the `git` binary is present in build environments; reference
the existing GitPython entry (`gitpython = ">=3.1.46,<4"`) and add an
appropriate `git` package declaration (matching your platform package naming
convention) so `source/collect_plugins.py` can safely call
`git.Repo.clone_from()` without an "executable not found" failure.
In `@source/collect_plugins.py`:
- Around line 12-14: There is a duplicate import of the module tempfile (import
tempfile appears twice); remove the redundant import so only a single "import
tempfile" remains (locate the duplicate import statement in collect_plugins.py
and delete the extra one to resolve the F811 duplicate-import error).
- Around line 349-358: The code creates tmpdir once then deletes it inside the
branch loop causing FileNotFoundError on subsequent attempts; fix by creating
and using a fresh temporary directory for each branch attempt (move
tempfile.mkdtemp() or use tempfile.TemporaryDirectory() inside the for branch in
branches loop), perform git.Repo.clone_from(repo_url, to_path=tmpdir, bare=True)
into that per-attempt directory, and ensure cleanup is done in a finally block
(or rely on TemporaryDirectory context) instead of unconditionally calling
shutil.rmtree(tmpdir) after each try so shutil.rmtree is only called for the
directory that was actually created.
- Line 344: The function signature for retrieve_plugin_markdown_files uses an
invalid annotation branches: [str] which is a list literal, not a type hint;
change the parameter annotation to a proper sequence type such as branches:
list[str] (or Sequence[str] if you prefer typing imports) in the
retrieve_plugin_markdown_files definition and update any related type hints or
references accordingly so static checkers and runtime semantics are correct.
- Around line 352-353: Validate and sanitize repo_url before calling
git.Repo.clone_from: parse repo_url (e.g., in the code surrounding the repo =
git.Repo.clone_from(repo_url, to_path=tmpdir, bare=True) call), verify the
scheme is exactly "https" and the hostname matches your allowed public hosts
list, and raise an exception or skip the package if validation fails; do this
check on the repo_url variable and only proceed to clone and use
repo.git.show(...) when the URL passes validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7932a77f-b819-4856-b6a7-0c5ffff1dd96
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pixi.tomlsource/collect_plugins.py
|
|
||
| SECTION_MARK_ORDER = '#*=-^"~:`_+<' | ||
|
|
||
| def retrieve_plugin_markdown_files(repo_url: str, branches:[str], section: str): |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
ns = {}
exec("def f(branches:[str]):\n pass\n", ns)
ann = ns["f"].__annotations__["branches"]
print(ann)
print(type(ann).__name__)
PYRepository: snakemake/snakemake-plugin-catalog
Length of output: 98
🏁 Script executed:
head -350 source/collect_plugins.py | tail -10Repository: snakemake/snakemake-plugin-catalog
Length of output: 355
Fix the branches annotation.
branches: [str] is not a valid type annotation; it evaluates to a list literal containing the str type object, not a list type hint. Use list[str] instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/collect_plugins.py` at line 344, The function signature for
retrieve_plugin_markdown_files uses an invalid annotation branches: [str] which
is a list literal, not a type hint; change the parameter annotation to a proper
sequence type such as branches: list[str] (or Sequence[str] if you prefer typing
imports) in the retrieve_plugin_markdown_files definition and update any related
type hints or references accordingly so static checkers and runtime semantics
are correct.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/collect_plugins.py (1)
360-360: Avoid mutable default argument.Using a mutable list as a default argument (
branches=["main", "master"]) is a Python anti-pattern. If the list were ever mutated, changes would persist across function calls.♻️ Suggested fix
-def get_docs(repository: str | None, section: str, branches=["main", "master"]): +def get_docs(repository: str | None, section: str, branches=None): if repository is None: return None + if branches is None: + branches = ["main", "master"] retrieved = retrieve_plugin_markdown_files(repository, branches, section)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/collect_plugins.py` at line 360, The function get_docs currently uses a mutable default argument branches=["main", "master"], which can lead to state leakage if mutated; change the signature to use branches: list[str] | None = None (or branches=None) and inside get_docs (the get_docs function) set branches = ["main", "master"] (or tuple("main","master")) when branches is None, ensuring callers still get the same defaults while avoiding a mutable default; update any type hints or callers if needed and do not mutate the default list in-place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/collect_plugins.py`:
- Line 360: The function get_docs currently uses a mutable default argument
branches=["main", "master"], which can lead to state leakage if mutated; change
the signature to use branches: list[str] | None = None (or branches=None) and
inside get_docs (the get_docs function) set branches = ["main", "master"] (or
tuple("main","master")) when branches is None, ensuring callers still get the
same defaults while avoiding a mutable default; update any type hints or callers
if needed and do not mutate the default list in-place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96de2463-571f-48a4-a0af-b76a337e5242
📒 Files selected for processing (2)
.github/workflows/deploy.ymlsource/collect_plugins.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
source/collect_plugins.py (2)
343-343:⚠️ Potential issue | 🟡 MinorFix the
branchesannotation on Line 343.
branches: [str]evaluates to a list literal, not a list type hint. Uselist[str]or another real sequence type instead.Proposed fix
-def retrieve_plugin_markdown_files(repo_url: str, branches: [str], section: str): +def retrieve_plugin_markdown_files(repo_url: str, branches: list[str], section: str):#!/bin/bash python - <<'PY' ns = {} exec("def f(branches:[str]):\n pass\n", ns) bad = ns["f"].__annotations__["branches"] print("bad:", repr(bad), type(bad).__name__) ns = {} exec("def f(branches:list[str]):\n pass\n", ns) good = ns["f"].__annotations__["branches"] print("good:", repr(good), type(good).__name__) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/collect_plugins.py` at line 343, The type annotation for the parameter branches in retrieve_plugin_markdown_files is using a list literal ([str]) instead of a proper type hint; change it to a real sequence type such as list[str] (or typing.List[str] / typing.Sequence[str] for older Python versions) and update any necessary imports (typing.List or typing.Sequence) so the annotation is a true type hint rather than a list value.
349-351:⚠️ Potential issue | 🟠 MajorAllowlist repository URLs before Line 351.
This helper now feeds the PyPI
RepositoryURL straight intoRepo.clone_from. That expands the trust boundary from public HTTPS fetches to whatever transportsgit cloneaccepts, including unexpected local or SSH targets. Reject anything except the HTTPS hosts you explicitly support before cloning.Proposed fix
def retrieve_plugin_markdown_files(repo_url: str, branches: list[str], section: str): """ fetch the intro.md and further.md doc files provided by plugins """ + from urllib.parse import urlparse + + parsed = urlparse(repo_url) + if parsed.scheme != "https" or parsed.hostname not in {"github.com", "gitlab.com"}: + print(f"Skipping unsupported repository URL for docs fetch: {repo_url}") + return None + docs_path = f"docs/{section}.md"Does GitPython `Repo.clone_from` shell out to the system `git clone`, and which repository URL schemes/transports can `git clone` access by default?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/collect_plugins.py` around lines 349 - 351, Validate and allowlist repo_url before calling git.Repo.clone_from: ensure repo_url uses HTTPS and the hostname is one of the supported hosts (reject ssh, file, git, or other schemes), returning or raising a clear error for disallowed URLs; perform this check where repo_url is passed to git.Repo.clone_from (in the helper using tempfile.TemporaryDirectory and variable repo_url), log the rejected URL and reason, and only proceed to clone when the allowlist check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/collect_plugins.py`:
- Around line 349-351: The with-statement uses the TemporaryDirectory class
instead of an instance, causing a TypeError; update the context manager to
instantiate it (use tempfile.TemporaryDirectory() in the with line) so that the
context yields a usable tmpdir path for git.Repo.clone_from(repo_url,
to_path=tmpdir, bare=True) in collect_plugins.py.
---
Duplicate comments:
In `@source/collect_plugins.py`:
- Line 343: The type annotation for the parameter branches in
retrieve_plugin_markdown_files is using a list literal ([str]) instead of a
proper type hint; change it to a real sequence type such as list[str] (or
typing.List[str] / typing.Sequence[str] for older Python versions) and update
any necessary imports (typing.List or typing.Sequence) so the annotation is a
true type hint rather than a list value.
- Around line 349-351: Validate and allowlist repo_url before calling
git.Repo.clone_from: ensure repo_url uses HTTPS and the hostname is one of the
supported hosts (reject ssh, file, git, or other schemes), returning or raising
a clear error for disallowed URLs; perform this check where repo_url is passed
to git.Repo.clone_from (in the helper using tempfile.TemporaryDirectory and
variable repo_url), log the rejected URL and reason, and only proceed to clone
when the allowlist check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0870031d-0d42-4f97-8f9f-903f8d30380f
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pixi.tomlsource/collect_plugins.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pixi.toml
fbartusch
approved these changes
Mar 9, 2026
fbartusch
left a comment
fbartusch
left a comment
There was a problem hiding this comment.
Tested the changes locally, the missing documentation (intro.md, further.md) is now included again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #23 .
Since GH became more agressive with its rate limiting, the collection of plugin docs would fail with a HTTP status code 429: Too many requests.
This PR moves the markdown fetching logic from plain http requests to using gitPython.
Test Plan
pixi run buildbuild/index.htmlfile and check thata. Plugins that provide 'intro.md' (e.g. the slurm executor plugin) have their documentation displayed
b. Pages that do not provide their own documentation show a little warning box
Summary by CodeRabbit
Refactor
Chores
Style